Skip to content

feat(helm): add node-data-broker ConfigMap mounts#347

Open
dmitsh wants to merge 1 commit into
mainfrom
ds-ib-config
Open

feat(helm): add node-data-broker ConfigMap mounts#347
dmitsh wants to merge 1 commit into
mainfrom
ds-ib-config

Conversation

@dmitsh

@dmitsh dmitsh commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Description

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • All commits are signed off per DCO (git commit -s).

@dmitsh dmitsh requested review from ravisoundar and resker June 10, 2026 21:26
@greptile-apps

greptile-apps Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a configMapMounts feature to the node-data-broker subchart, allowing users to declare ConfigMaps inline in values and have them automatically created and mounted into both the init container and the main container — primarily to supply extra config files (e.g. ibdiag.conf for ibnetdiscover).

  • configmap-mounts.yaml — new template that renders one ConfigMap per configMapMounts[] entry using a |- block scalar for each data key; controlled by the same enabled guard as the DaemonSet.
  • daemonset.yamlvolumeMounts and volumes sections extended to include the generated ConfigMaps in both init and main containers, keeping backward compatibility with the existing volumes/volumeMounts values fields.
  • _helpers.tpl — two new helpers (configMapMountName, configMapMountVolumeName) derive K8s-safe names from the user-supplied name field; chart test coverage added in scripts/chart-test.sh.

Confidence Score: 5/5

Safe to merge — the missing ConfigMap template identified in the previous review is now present, the naming helpers are consistent between the ConfigMap and DaemonSet volume references, and all chart-test assertions should pass.

The configmap-mounts.yaml template correctly creates one ConfigMap per entry, the DaemonSet consistently references those ConfigMaps by the same generated names, the enabled guard is applied in both templates, and the chart test exercises the full rendering path including data content, mountPath, and subPath.

No files require special attention. The configmap-mounts.yaml data-block rendering uses a hand-rolled indent-4 approach rather than the more common nindent pattern, but it produces correct YAML for all documented use cases.

Important Files Changed

Filename Overview
charts/topograph/charts/node-data-broker/templates/configmap-mounts.yaml New template rendering one ConfigMap per configMapMounts entry; data block uses a custom indent-4 approach that produces valid YAML for the documented use cases.
charts/topograph/charts/node-data-broker/templates/daemonset.yaml Extends volumeMounts and volumes sections to include configMapMounts; correctly scoped inside the existing enabled guard and mirrors configMapMountName/VolumeName helpers consistently.
charts/topograph/charts/node-data-broker/templates/_helpers.tpl Two new helpers for deriving ConfigMap and volume names; follows established trunc-63/trimSuffix pattern; uses required to enforce name presence.
charts/topograph/charts/node-data-broker/values.yaml Adds configMapMounts key (defaulting to empty list) with inline example; enabled defaults to true so chart tests work without extra flags.
scripts/chart-test.sh New test case exercises configMapMounts via --set-json; assertions check ConfigMap name, data content indentation, mountPath, and subPath in rendered output.
docs/providers/infiniband.md Documents the new configMapMounts feature with an ibdiag.conf example; straightforward documentation addition.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[values.yaml\nconfigMapMounts list] --> B{enabled AND\nconfigMapMounts non-empty?}
    B -- Yes --> C[configmap-mounts.yaml\nrange over configMapMounts]
    C --> D[Render ConfigMap per entry\nname via configMapMountName helper\ndata keys rendered as block scalars]
    B -- No --> E[No ConfigMaps rendered]
    A --> F{daemonset.yaml\nenabled?}
    F -- Yes --> G[DaemonSet spec]
    G --> H{configMapMounts or\nvolumeMounts set?}
    H -- Yes --> I[volumeMounts in init container\nvolumeMounts in main container]
    H -- No --> J[No volumeMounts block]
    G --> K{configMapMounts or\nvolumes set?}
    K -- Yes --> L[Pod volumes\nconfigMap.name via configMapMountName\nvolumeName via configMapMountVolumeName]
    K -- No --> M[No volumes block]
    D -.->|names match| L
Loading

Reviews (2): Last reviewed commit: "feat(helm): add node-data-broker ConfigM..." | Re-trigger Greptile

Comment thread charts/topograph/charts/node-data-broker/templates/daemonset.yaml
Comment thread scripts/chart-test.sh
@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.80%. Comparing base (1875ab8) to head (01ad8b6).
⚠️ Report is 68 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
+ Coverage   68.46%   70.80%   +2.33%     
==========================================
  Files          82       84       +2     
  Lines        4842     5031     +189     
==========================================
+ Hits         3315     3562     +247     
+ Misses       1395     1304      -91     
- Partials      132      165      +33     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
@github-actions

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant